Skip to content

[SYCL][Graph] Modified the adapters such that it is valid to call release on... #18619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: sycl
Choose a base branch
from

Conversation

konradkusiak97
Copy link
Contributor

Command Buffer, while it is still executing.

@konradkusiak97 konradkusiak97 requested a review from a team as a code owner May 22, 2025 10:50
@konradkusiak97 konradkusiak97 requested a review from EwanC May 22, 2025 10:50
@@ -1175,6 +1178,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueCommandBufferExp(
if (phEvent) {
UR_CHECK_ERROR(RetImplEvent->record());
*phEvent = RetImplEvent.release();
hCommandBuffer->CurrentExecution = *phEvent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this solution work if the UR caller doesn't pass a phEvent to urEnqueueCommandBufferExp, and then does a command-buffer release? Note, this is what your CTS test does.

Copy link
Contributor Author

@konradkusiak97 konradkusiak97 May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. It seems for CUDA and HIP we'd need to create the event regardless if SYCL RT asked for it or not and then cache it in the command-buffer. I can't really see any other option 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I can't think of another solution either. Can you check if we have wording in the sycl_ext_oneapi_graph spec about this behavior on executable graph destruction? If we're doing extra work ontop of backends to give users a nicer experience then we should at least advertise that. I'm guessing we have SYCL e2e tests already that check this, since I think you said you saw some fail when the SYCL-RT code was updated but not the UR code.

Copy link
Contributor Author

@konradkusiak97 konradkusiak97 May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if we have wording in the sycl_ext_oneapi_graph spec about this behavior on executable graph destruction?

I don't think we're mentioning anything about this in the spec.

If we're doing extra work ontop of backends to give users a nicer experience then we should at least advertise that.

Ok, I'll add something to the spec about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants